not critical / minor: tbh, it seems a bit out of place for the test_save_best_checkpoint (as well as the previous case). I would probably move it into a separate test. Or should it otherwise call at least train and test actual checkpoint saved?

I guess I agree that it logically does seem a bit out of place. I think cases 3 and 4 could be grouped into their own methods since the point isn't so much to test the save_strategy = "best" itself but more to test the behavior related to metric_for_best_model.

I'm not sure if actually running training would be necessary, though. Case 3 is simply to check whether a ValueError is being properly thrown at Trainer initialization time, and case 4 is also simply to check whether the __post_init__ method of TrainingArguments properly initializes metric_for_best_model to "loss" when save_strategy != "best" and load_best_model_at_end = True. To me, neither of these seem to require training/evaluation and Trainer instantiation seems sufficient.

Agreed, I would also split it into a separate test (or two test). And, yes, we are testing the init here, that's why it was looking out of place.

no strong opinion. We can split it into a separate test for case 3 and 4.

my 2cs (Disclaimer! I'm not very familiar with the whole scope of the initial change, or reason behind it!): it's a bit hard to read and understand what is going on here and why. E.g. why can't it default to loss when save_strategy == best? What is the major difference with the load_best_model_at_end (and save_strategy!="best")?

Again, apologies if I'm missing some obvious context here. Please feel free to disregard my comment / question then.

I didn't find the place where we set metric_for_best_model = "loss" when save_strategy!=best. Can you explain a bit why you changed the description ?

@shcheklein That was a design decision made here (#31817 (comment)). It was deemed easier to debug if we don't add a hard-coded value and rather raise an error.

@SunMarc Hmm I'm starting to think that maybe the problem is that we're not able to set load_best_model_at_end = True when save_strategy = "best" since load_best_model_at_end requires eval_strategy == save_strategy but eval_strategy doesn't have a "best" option.

This means that if we want to use save_strategy = "best" then we have to have load_best_model_at_end = False, which in turn means that when save_strategy != "best" and load_best_model_at_end = True then the __post_init__ method of TrainingArguments is setting metric_for_best_model to "loss". https://github.com/huggingface/transformers/blob/main/src/transformers/training_args.py#L1676:L1679

The modified docstring aims to add a bit more detail as to when the metric_for_best_model is set to a default of "loss".

should we also add best for eval_strategy then ?

I feel like that might sound a bit awkward since it means we'd "perform evaluation at every best checkpoint."

Maybe we could check if save_strategy == "best" and then bypass the eval_strategy == save_strategy condition? That would mean that here we would change the code to:

if self.load_best_model_at_end and self.save_strategy != "best":
    if self.eval_strategy != self.save_strategy:
        raise ValueError(
            "--load_best_model_at_end requires the save and eval strategy to match, but found\n- Evaluation "
            f"strategy: {self.eval_strategy}\n- Save strategy: {self.save_strategy}"
        )

I'm not 100% sure about the history of why eval_strategy == save_strategy but I'm assuming that it's a safe guard to prevent situations where we want to load the best model at the end of training but we never saved it because the two didn't match. If save_strategy == "best" I don't think we'd have that problem since saving is guaranteed to following evaluation.

I think this also means that we may have to either remove the default loss error we're raising or change it to a warning (#31817 (comment)).

Sounds good to me for the new condition, just add a comment to explain why we don't need to perform the check when self.save_strategy == "best". Also, which default loss error you are talking about ? I'm not sure why we need to remove it.

I was referring to this block of code:

class Trainer:
    def __init__(...):
        ...

        if args.save_strategy == SaveStrategy.BEST or args.load_best_model_at_end:
            if args.metric_for_best_model is None:
                raise ValueError(
                    "`args.metric_for_best_model` must be provided when using 'best' save_strategy or if `args.load_best_model_at_end` is set to `True`."
                )

I believe that this error was added because rather than allowing the metric_for_best_model to default to loss, it would be easier to debug if we force the user to explicitly set a loss.

But if we change the __post_init__ method of TrainingArguments, the metric_for_best_model would default to loss if load_best_model_at_end == True regardless of the value of save_strategy.

I don't think we have to remove the error message, but I just thought that it would be a bit different from what we initially intended.

no strong opinion. We can split it into a separate test for case 3 and 4.

I didn't find the place where we set metric_for_best_model = "loss" when save_strategy!=best. Can you explain a bit why you changed the description ?